fix(swift-sdk): fix dash-spv sync (somehow)#3404
Conversation
📝 WalkthroughWalkthroughSPV synchronization methods were made synchronous: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "8b4c7ecfa04d142f8a405085177f6a0877d076adae22f0ac1c738b7b1d945ab0"
)Xcode manual integration:
|
|
Heya! I need recent version of dashcore here #3405. I guess it affect what you are doing here |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Neither proposed finding is verified against the source. The startSync signature change is not a proven source-breaking compile failure in Swift, and the SPV directory regression is only a suspicion because the native FFI directory-creation behavior is not present in this repo for verification.
Reviewed commit: 9f8ec47
ZocoLini
force-pushed
the
fix/dash-spv-sync
branch
from
9f8ec47 to
f662bc9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift`:
- Around line 327-329: The call path currently invokes WalletService.startSync()
(which is `@MainActor`) and ultimately SPVClient.startSync() that calls the
synchronous FFI dash_spv_ffi_client_run(), blocking the main thread; fix by
offloading the blocking FFI call to a background queue—either wrap the
spvClient.startSync() invocation inside DispatchQueue.global().async inside
WalletService.startSync() (clearing lastSyncError before dispatch and setting
lastSyncError back on the MainActor on error), or move the
DispatchQueue.global().async into SPVClient.startSync() around the
dash_spv_ffi_client_run() call so that the synchronous FFI never runs on the
main thread. Ensure errors are propagated back to the main actor (e.g., update
lastSyncError on MainActor) and keep caller API unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b167915-df7b-46a9-b21d-a6b78991b740
📒 Files selected for processing (3)
packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swiftpackages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/swift-sdk/Sources/SwiftDashSDK/Core/Services/WalletService.swift
- packages/swift-sdk/Sources/SwiftDashSDK/Core/SPV/SPVClient.swift
| private func startSync() { | ||
| Task { | ||
| await walletService.startSync() | ||
| } | ||
| walletService.startSync() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WalletService isolation and startSync body =="
fd -a 'WalletService.swift' | while read -r f; do
echo "FILE: $f"
rg -n -C3 '@MainActor|public func startSync\(|spvClient.startSync\(' "$f"
done
echo
echo "== SPVClient startSync and FFI run call =="
fd -a 'SPVClient.swift' | while read -r f; do
echo "FILE: $f"
rg -n -C3 'func startSync\(|dash_spv_ffi_client_run' "$f"
done
echo
echo "== Any explicit background offloading around sync start =="
rg -n --type=swift -C2 'Task\.detached|DispatchQueue\.global|withCheckedContinuation|startSync\(' packages/swift-sdkRepository: dashpay/platform
Length of output: 45226
UI-thread blocking risk: startSync() invokes FFI without background offloading.
Line 328 calls walletService.startSync() directly from the main-thread action path. WalletService.startSync() is @MainActor and invokes spvClient.startSync(), which directly calls the synchronous FFI dash_spv_ffi_client_run() without dispatching to a background queue. This breaks the established SDK pattern: similar heavy FFI operations (e.g., in ShieldedPoolClient, AddressSyncService, ShieldedCryptoService) consistently offload work via DispatchQueue.global().async to prevent main-thread blocking. SPV synchronization is a blocking I/O operation and must be moved off the main thread.
Wrap the FFI call in DispatchQueue.global().async within SPVClient.startSync(), or refactor WalletService.startSync() to dispatch the call asynchronously:
public func startSync() {
lastSyncError = nil
DispatchQueue.global().async {
do {
try self.spvClient.startSync()
} catch {
Task { `@MainActor` in
self.lastSyncError = error
print("❌ Sync failed: \($0)")
}
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/CoreContentView.swift`
around lines 327 - 329, The call path currently invokes
WalletService.startSync() (which is `@MainActor`) and ultimately
SPVClient.startSync() that calls the synchronous FFI dash_spv_ffi_client_run(),
blocking the main thread; fix by offloading the blocking FFI call to a
background queue—either wrap the spvClient.startSync() invocation inside
DispatchQueue.global().async inside WalletService.startSync() (clearing
lastSyncError before dispatch and setting lastSyncError back on the MainActor on
error), or move the DispatchQueue.global().async into SPVClient.startSync()
around the dash_spv_ffi_client_run() call so that the synchronous FFI never runs
on the main thread. Ensure errors are propagated back to the main actor (e.g.,
update lastSyncError on MainActor) and keep caller API unchanged.
|
Everything looks to be working now |
3 participants
Somehow this Pr makes dash-spv work in my machine after clearing the simulator with the changes this PR introduce, I didn't do a deep research on why, I will be investigating tomorrow, I have a little theory and has too do with file permissions and the way FileManager creates directories vs how Rust does. The reason I think thats the issue its bcs sync was working fine for me until I cleared the storage, then it got stuck until I removed the file creation and created a new simulator, letting dash-spv manage the directory creation
Summary by CodeRabbit